-
Notifications
You must be signed in to change notification settings - Fork 47
Adds uncertainties to Invariant inputs #3835
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Allows calculation if no eror input Resizes UI Proper rebase, UI and uncertainties correction Fixes tabstops for UI
Co-authored-by: Copilot <[email protected]>
DrPaulSharp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good so far, but I've found a few issues, alongside the specific comments.
- When entering a value in the contrast error box and then clearing it, it is marked as invalid.
- I can type letters into the Porod constant and error boxes (where they are correctly marked as invalid), but I am not able to do so in the contrast or volume fraction boxes. It would be good to replicate this functionality.
- When I have a value in the contrast box and then switch to volume fraction, the calculate button is disabled as there is no volume fraction, but it stays disabled when I switch back to contrast.
Also, please rebase this branch when the corfunc slider branch is merged, some of the functionality there is replicated here.
| self._volfrac2: float | None = None | ||
| self._volfrac1_err: float | None = None | ||
|
|
||
| # Old extrapolation parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what sense are these "old" extrapolation parameters, compared to the new ones below? Are both sets required?
| self.lblTotalQUnits.setStyleSheet(new_font) | ||
| self.lblPorodCstUnits.setStyleSheet(new_font) | ||
| self.lblContrastUnits.setStyleSheet(new_font) | ||
| self.lblContrastUnits_2.setStyleSheet(new_font) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you think of a better choice of variable name here?
| if self.model.item(WIDGETS.W_CONTRAST).text() != "None" and self.model.item(WIDGETS.W_CONTRAST).text() != "": | ||
| self._contrast = float(self.model.item(WIDGETS.W_CONTRAST).text()) | ||
| if ( | ||
| self.model.item(WIDGETS.W_CONTRAST_ERR).text() != "None" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you still need to test for it being "None"? How does this case arise in the current code?
| self._porod = float(self.model.item(WIDGETS.W_POROD_CST).text()) | ||
| if ( | ||
| self.model.item(WIDGETS.W_POROD_CST_ERR).text() != "None" | ||
| and self.model.item(WIDGETS.W_POROD_CST_ERR).text() != "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
| and $R_g$ are obtained by fitting the data within the range $q_{min}$ to | ||
| $q_{min+j}$ where $j$ is the user-chosen number of points from which to | ||
| extrapolate. The default is the first 10 points. Alternatively a power | ||
| extrapolate. The default is the lower 15\% of the data points. Alternatively a power |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify here? Is it the lower 15% of the data range, or literally 15% of the number of points?
| (\Delta\rho)^2 = \frac{Q^*}{2 \pi^2 \phi_1 \phi_2} | ||
| where $\phi_1$ is the known volume fraction of the minority phase, and $\phi_2 = | ||
| 1 - \phi_1$. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, does this mean that we should restrict phi to be less than 0.5 in the code? Maybe it's better to say something like phi_1 is by convention the minority phase.
| Switch to the *Extrapolation* tab. | ||
|
|
||
| Adjust the extrapolation types as necessary by checking the relevant check boxes. | ||
| If power law extrapolations are chosen, the exponent can be either held fixed or fitted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being particularly pernickety here, but I think ". . .the exponent can be either fitted or held fixed." reads a little better. It also matches the order in the extrapolation tab.
|
|
||
| Adjust the extrapolation types as necessary by checking the relevant check boxes. | ||
| If power law extrapolations are chosen, the exponent can be either held fixed or fitted. | ||
| The points over which the extrapolations are fitted can also be adjusted by changing the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to say "The range over which . . ." here, since a q value is entered, which may or may not lie on a point in the data.
Description
Uncertainty Propagation
This PR extends the Invariant perspective to support uncertainty propagation, covering both the core invariant calculations and the GUI workflow.
Uncertainties from input quantities (contrast, volume fraction, Porod constant) are propagated analytically and combined in quadrature where applicable.
Adds explicit uncertainty input fields for contrast, volume fraction, and porod constant
Documentation & tests
Adds documentation describing the invariant refactor and uncertainty derivations.
Adds unit tests covering uncertainty propagation.
Fixes #3813
How Has This Been Tested?
Review Checklist:
[if using the editor, use
[x]in place of[ ]to check a box]Documentation (check at least one)
Installers
Licensing (untick if necessary)